<html>
<head><meta charset="utf-8"><title>go to definition feature · rustdoc · Zulip Chat Archive</title></head>
<h2>Stream: <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/index.html">rustdoc</a></h2>
<h3>Topic: <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html">go to definition feature</a></h3>

<hr>

<base href="https://rust-lang.zulipchat.com">

<head><link href="https://rust-lang.github.io/zulip_archive/style.css" rel="stylesheet"></head>

<a name="243896872"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243896872" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> GuillaumeGomez <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243896872">(Jun 25 2021 at 10:11)</a>:</h4>
<p>Hi everyone,</p>
<p><a href="https://github.com/rust-lang/rust/pull/84176">https://github.com/rust-lang/rust/pull/84176</a> has been open for quite some time now and is at a dead point. The two arguments against it are:</p>
<ul>
<li>scope expansion (this is the biggest concern)</li>
<li>maintenability</li>
</ul>
<p>(Please correct me if I'm wrong <span class="user-mention" data-user-id="132040">@Manish Goregaokar</span> <span class="user-mention" data-user-id="232545">@Joshua Nelson</span>).</p>
<p>With the (great!) help of @jsha, I've put together a document to try to have a more clear view on this feature, what it implies, how it works and where we should put limits on additions to this feature as well. This last point is however very open to debate because it seems very tricky.  The goal is to try to sum up everything we said until now in order to (ideally) reach a consensus.</p>
<p><a href="https://gist.github.com/GuillaumeGomez/66bc7c6424dfe8ffe70739d5634e7f97">https://gist.github.com/GuillaumeGomez/66bc7c6424dfe8ffe70739d5634e7f97</a></p>
<p>So what do you think of this proposal?</p>
<p>cc <span class="user-mention" data-user-id="311797">@Oliver Middleton</span>, <span class="user-mention" data-user-id="210267">@Nemo157</span>, <span class="user-mention" data-user-id="307537">@Noah Lev</span>, <span class="user-mention" data-user-id="319144">@CraftSpider</span>, <span class="user-mention" data-user-id="315412">@jsha</span></p>



<a name="243927392"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243927392" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Manish Goregaokar <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243927392">(Jun 25 2021 at 14:59)</a>:</h4>
<p>So firstly , thanks for writing this. I think in general for big features like this I would prefer to have such proposals (though not necessarily _as_ in depth, just a plan in an issue is enough) _first_ before implementation work happens</p>



<a name="243927863"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243927863" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> GuillaumeGomez <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243927863">(Jun 25 2021 at 15:03)</a>:</h4>
<p>Sorry about that. It was just so clear in my mind that I didn't take all the non-technical parts into consideration. Only after you and <span class="user-mention" data-user-id="232545">@Joshua Nelson</span> strongly advising to look into non technical parts, I slowly started to wonder what I was missing. I'm ashamed to say so but <span class="user-mention" data-user-id="315412">@jsha</span> helped me to fully understand what I was missing. Now I see better, and I understand why you were so careful about it,  and i'm sorry for being so slow to see it.</p>
<p>The big issue now is to kinda say where we should put the limit. I'm not even sure if something like this is possible. It's hard to predict the future.</p>



<a name="243928178"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243928178" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Manish Goregaokar <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243928178">(Jun 25 2021 at 15:05)</a>:</h4>
<p>thanks</p>



<a name="243928409"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243928409" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Manish Goregaokar <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243928409">(Jun 25 2021 at 15:07)</a>:</h4>
<p>I think the thing <span class="user-mention" data-user-id="315412">@jsha</span> brings up about JTD for variables is on point and is exactly the kind of scope expansion I was afraid of. I think many users will consider this feature incomplete without JTD for variables because they'll expect it to work. Which is fine, but it's more design work and could potentially significantly complicate the code. Personally I'd prefer to not do JTD for variables but otoh i do feel like if we're going to do a thing we should do it right.</p>
<p>Though honestly, my biggest concern remains maintainability (of course scope expansion makes maintainability harder :P). I had basically said that I'd be okay with it if others do the review and think it's maintainable, but so far that hasn't happened, and jyn has also stepped away from reviewing this. When I tried to review this it was definitely not easy to understand/review which means that I'm still pretty concerned about that. It's possible that there's a way to implement this feature that's easy to review, but I don't think we're there yet, and the fact that nobody is reviewing this is a signal to me.</p>



<a name="243936621"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243936621" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> GuillaumeGomez <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243936621">(Jun 25 2021 at 16:07)</a>:</h4>
<p>Hum interesting, I'd personally not include JTD for variables but it's not much harder to add.</p>
<p>As for the maintenability, I'm surprised of your comment. <span class="user-mention" data-user-id="311797">@Oliver Middleton</span> reviewed it a few times already after all, and so did <span class="user-mention" data-user-id="232545">@Joshua Nelson</span> before being mostly not available (however he expressed concerns about the scope expansion).</p>
<p>If you take a look at the code of the PR: <a href="https://github.com/rust-lang/rust/pull/84176/files#diff-00b3b34ce2eb95c63cc6b1a4648ab3939af4b083e2fb0e8d199535fe239b576f">https://github.com/rust-lang/rust/pull/84176/files#diff-00b3b34ce2eb95c63cc6b1a4648ab3939af4b083e2fb0e8d199535fe239b576f</a> , it's 200 lines and most of it is documentation.</p>
<p>In <a href="https://github.com/rust-lang/rust/pull/84176/files#diff-eaff3f0a46e1d30c3ec7dc4a9ca5cdad2eb6e27b9fe1463bac5a1c928c94140a">https://github.com/rust-lang/rust/pull/84176/files#diff-eaff3f0a46e1d30c3ec7dc4a9ca5cdad2eb6e27b9fe1463bac5a1c928c94140a</a> we have ~230 lines in order to generate links into the source code files (and more than 40 lines of doc comments).</p>
<p>In <a href="https://github.com/rust-lang/rust/pull/84176/files#diff-04002bbcf90fc0a3356db4344dee8f1e03d868fa6c0f666713affa7fa2679b6f">https://github.com/rust-lang/rust/pull/84176/files#diff-04002bbcf90fc0a3356db4344dee8f1e03d868fa6c0f666713affa7fa2679b6f</a> I created a source file collector because I use it in the new visitor (to create the span map) as well to prevent doing it more than necessary.</p>
<p>We have more than 100 lines of tests.</p>
<p>The rest is about passing down the parameter to the inside of rustdoc and extra documentation.</p>
<p>This is far from the first time where a big rust(doc) PR takes time to get reviewed. I don't see this as an issue but more as a precaution. However I proposed multiple times to help with the review and answer any questions that might appear. For me there is no big technical changes, but I'm the one who wrote it, so...</p>
<p>If anyone is willing to take a few hours to make a call so we can go through the PR together, I don't mind at all. It's a big PR with at least 300 lines of newly added code after all. So if I can do anything to make the review process simpler for you, please tell me.</p>



<a name="243944444"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243944444" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Joshua Nelson <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243944444">(Jun 25 2021 at 17:14)</a>:</h4>
<p>On the technical side, my concern is almost always about implicit state between different parts of rustdoc. This changes the following pieces of implicit state:</p>
<ul>
<li>emit_sources is moved from SharedContext to Context. I am slightly concerned this makes it easy for it to get out of sync when the context is cloned, but not enough to block on it.</li>
<li>the pass assumes the span map was filled out earlier. I don't see any way around this, all of rustdoc works like this currently and I would feel bad about holding this to a higher standard than the existing code. I do think we as a team should sit down sometime and think up ways to do things on demand.</li>
</ul>
<p>Overall it's a lot better than I remember and, ignoring the scope issue, I would be ok with merging it as-is.</p>



<a name="243945057"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243945057" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Joshua Nelson <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243945057">(Jun 25 2021 at 17:19)</a>:</h4>
<p>Also <span class="user-mention" data-user-id="132040">@Manish Goregaokar</span> I haven't had time for <em>any</em> reviews, not just this one, I don't want that to be a point against it</p>



<a name="243952053"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243952053" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> GuillaumeGomez <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243952053">(Jun 25 2021 at 18:20)</a>:</h4>
<p>The cleanup is paused for the moment: on my side, I need <span class="user-mention" data-user-id="232545">@Joshua Nelson</span> to review these parts of rustdoc, even if not as "main" reviewer but I feel more confident having him take a look.</p>
<p>As for the technical issues listed, it's exactly as you said: that will require a huge refactor in any case. I hope we'll get to it at some point.</p>
<p>So now remains to agree on the scope issue or more precisely: how to put limits. I don't know how to word such a thing precisely or even how to categorize some features as "unwanted" and some others as "acceptable".</p>



<a name="243986462"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243986462" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Manish Goregaokar <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243986462">(Jun 26 2021 at 01:04)</a>:</h4>
<p><span class="user-mention" data-user-id="210316">@GuillaumeGomez</span> Sorry! I hadn't noticed that ollie had reviewed this. The new code does look better. Still a bit concerned but my main issue was that I'd be okay with it if the reviewer felt that maintainability was resolved and it seems like that's the case, so I'm fine with it.</p>
<p>A question is: putting aside whether or not we should be doing JTD for vars, how much more complicated do you forsee the implementation getting if we did? If the answer is "not much at all" then we're probably fine, otherwise we have to think about scoping.</p>
<p><span class="user-mention" data-user-id="232545">@Joshua Nelson</span> yeah I know you haven't had time for reviews, but the fact that it took so long to get a review for in general (and had multiple false starts where I started to review but it took too much time to even understand what was happening) was a signal for me</p>



<a name="243986576"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/243986576" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Joshua Nelson <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#243986576">(Jun 26 2021 at 01:06)</a>:</h4>
<p>I still feel like the <code>cfg(doc)</code> thing has not really been addressed; that's a fragile part of rustdoc and <em>regularly</em> gets special-cased in the compiler proper. Trying to type check bodies is tempting fate. (see also <a href="https://github.com/rust-lang/rfcs/pull/2963#issuecomment-669515931">https://github.com/rust-lang/rfcs/pull/2963#issuecomment-669515931</a>)</p>



<a name="244002082"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/244002082" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Manish Goregaokar <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#244002082">(Jun 26 2021 at 08:12)</a>:</h4>
<p>Yeah, that's a big reason to avoid looking at bodies. In general I am more comfy looking at signatures over bodies but this is the kind of scope expansion i was worried about</p>



<a name="244003601"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/244003601" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> GuillaumeGomez <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#244003601">(Jun 26 2021 at 08:49)</a>:</h4>
<p><span class="user-mention silent" data-user-id="132040">Manish Goregaokar</span> <a href="#narrow/stream/266220-rustdoc/topic/go.20to.20definition.20feature/near/243986462">said</a>:</p>
<blockquote>
<p>A question is: putting aside whether or not we should be doing JTD for vars, how much more complicated do you forsee the implementation getting if we did? If the answer is "not much at all" then we're probably fine, otherwise we have to think about scoping.</p>
</blockquote>
<p>Not much at all. I simply didn't want to put more into this PR to keep it simpler to review. Most of the changes come from how we generate links into the source code pages, not from how we generate the span map. If we want to add this feature as well, I can add it into a follow-up PR.</p>
<p><span class="user-mention silent" data-user-id="132040">Manish Goregaokar</span> <a href="#narrow/stream/266220-rustdoc/topic/go.20to.20definition.20feature/near/243986462">said</a>:</p>
<blockquote>
<p><span class="user-mention silent" data-user-id="232545">Joshua Nelson</span> yeah I know you haven't had time for reviews, but the fact that it took so long to get a review for in general (and had multiple false starts where I started to review but it took too much time to even understand what was happening) was a signal for me</p>
</blockquote>
<p>At the time the PR was facing a bit more troubles because we came across <a href="https://github.com/rust-lang/rust/pull/84961">this bug</a>. I fixed it in <a href="https://github.com/rust-lang/rust/pull/84953">this PR</a>, making the PR lose a huge part of its code. Another thing to take into account is that this is a rustdoc part that not much rustdoc reviewers know. We are all "specialized", and I think only <span class="user-mention" data-user-id="311797">@Oliver Middleton</span> and <span class="user-mention" data-user-id="232545">@Joshua Nelson</span> are able to review this one "confidently". Also, you said yourselves that you were busy, and you didn't contribute on rustdoc source code since quite some time now. So maybe on your side, it'd take more time to remember the whole rustdoc structure, making the review more complicating, discouraging you?</p>
<p><span class="user-mention silent" data-user-id="232545">Joshua Nelson</span> <a href="#narrow/stream/266220-rustdoc/topic/go.20to.20definition.20feature/near/243986576">said</a>:</p>
<blockquote>
<p>I still feel like the <code>cfg(doc)</code> thing has not really been addressed; that's a fragile part of rustdoc and <em>regularly</em> gets special-cased in the compiler proper. Trying to type check bodies is tempting fate. (see also <a href="https://github.com/rust-lang/rfcs/pull/2963#issuecomment-669515931">https://github.com/rust-lang/rfcs/pull/2963#issuecomment-669515931</a>)</p>
</blockquote>
<p>I already started asking some compiler people about this. They didn't seem very worried about that though... So no idea if that's a big issue or not. I'd argue that the fact this feature it's nightly only and will remain until we're sure all issues are fixed and the scope is defined should put us more at ease but I understand your worries.</p>



<a name="244014177"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/244014177" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Joshua Nelson <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#244014177">(Jun 26 2021 at 13:05)</a>:</h4>
<p><span aria-label="+1" class="emoji emoji-1f44d" role="img" title="+1">:+1:</span>  I am ok with this if it's nightly only</p>



<a name="244018916"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/266220-rustdoc/topic/go%20to%20definition%20feature/near/244018916" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> GuillaumeGomez <a href="https://rust-lang.github.io/zulip_archive/stream/266220-rustdoc/topic/go.20to.20definition.20feature.html#244018916">(Jun 26 2021 at 14:56)</a>:</h4>
<p>It'll remain so for some time. I want at least you and @jsha to give approval on the code and on the front-end "look" before stabilization. So we have (at least/minimum) a few months ahead of us.</p>



<hr><p>Last updated: Aug 07 2021 at 22:04 UTC</p>
</html>